-
Notifications
You must be signed in to change notification settings - Fork 20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve PR search when gitarro is running with cron-like tools to avoid exceeding GitHub API limits #47
Conversation
In the future we can also consider using Conditional Requests. At this moment I didn't find how to use them with octokit. |
And just as an example of a test hitting the limit: https://travis-ci.org/openSUSE/gitarro/builds/280934686?utm_source=github_status&utm_medium=notification :-) We now need to wait so the test can pass. |
12f4569
to
2a986e9
Compare
@juliogonzalez lthx ooks really cool. i will review it with calm tomorrow |
@@ -84,8 +90,7 @@ def retrigger_check(pr) | |||
|
|||
# public always rerun tests against the pr number if this exists | |||
def trigger_by_pr_number(pr) | |||
return false if @pr_number.nil? | |||
return false if @pr_number != pr.number | |||
return false if @pr_number.nil? || @pr_number != pr.number |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small refactor to avoid rubocop complains.
lib/gitarro/backend.rb
Outdated
@@ -117,8 +122,7 @@ def reviewed_pr_test(comm_st, pr) | |||
pr_all_files_type(@repo, pr.number, @file_type) | |||
return true if changelog_active(pr, comm_st) | |||
return false unless @pr_files.any? | |||
exit 1 if @check | |||
launch_test_and_setup_status(@repo, pr) | |||
@check ? exit(1) : launch_test_and_setup_status(@repo, pr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small refactor to avoid rubocop complains.
lib/gitarro/backend.rb
Outdated
# a pr contain always a comments, cannot be nil | ||
pr_comment.each do |com| | ||
@client.issue_comments(repo, pr_number).each do |com| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small refactor to avoid rubocop complains.
lib/gitarro/backend.rb
Outdated
@@ -160,8 +163,7 @@ def magicword(repo, pr_number, context) | |||
# check all files of a Prs Number if they are a specific type | |||
# EX: Pr 56, we check if files are '.rb' | |||
def pr_all_files_type(repo, pr_number, type) | |||
files = @client.pull_request_files(repo, pr_number) | |||
files.each do |file| | |||
@client.pull_request_files(repo, pr_number).each do |file| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small refactor to avoid rubocop complains.
lib/gitarro/backend.rb
Outdated
@@ -259,8 +261,7 @@ def do_changelog_test(repo, pr) | |||
def changelog_changed(repo, pr, comm_st) | |||
return false unless @changelog_test | |||
# only execute 1 time, don"t run if test is failed, or ok | |||
return false if failed_status?(comm_st) | |||
return false if success_status?(comm_st) | |||
return false if failed_status?(comm_st) || success_status?(comm_st) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small refactor to avoid rubocop complains.
2a986e9
to
33133da
Compare
tests/unit_tests/t_backend.rb
Outdated
end | ||
|
||
def test_get_no_prs | ||
@full_hash[:repo] = 'openSUSE/gitbot' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo gitbot --> gitarro
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
tests/unit_tests/t_backend.rb
Outdated
@@ -48,4 +49,19 @@ def assert_file_non_ex(gbex, test_file) | |||
assert_equal("'#{test_file}\' doesn't exists.Enter valid file, -t option", | |||
ex.message) | |||
end | |||
|
|||
def test_get_all_prs | |||
@full_hash[:repo] = 'openSUSE/gitbot' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gitarro
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
33133da
to
f357508
Compare
gitarro.rb
Outdated
prs = b.open_newer_prs | ||
|
||
# Exit without errors if no PRs were found | ||
if prs.empty? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
even if rubocop will complain, is good to move this function to a class to backend.rb.
I would put it in the general class, or we could create a module which is included by the main class of backend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, maybe we should call it inside Backend.open_newer_prs() and exit if the array is empty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yo
lib/gitarro/backend.rb
Outdated
def open_newer_prs | ||
prs = [] | ||
@client.pull_requests(@repo, state: 'open').each do |pr| | ||
if Time.now.utc - pr.updated_at < @options[:change_newer] * 60 || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we need here a private method for 2 reasons:
something like: ( naming could be improved)
def pr_comment_commit_changed_less_then(pr, min)
return true if Time.now.utc - pr.updated_at < @options[:change_newer] * 60 || ..
end
- for making the code more abstract from the algorithm itself, and more maintenable.
- it will remove the the if and end block, making a inline if (more compact)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, but maybe pr_changed_less_than(pr, min) will be a easier name. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yop, but if so i would make a comment above the function saying what trigger tje change
lib/gitarro/opt_parser.rb
Outdated
change_newer_desc = 'If present, will only check PRs with a change in ' \ | ||
'the last X minutes' | ||
opt.on("--change_newer 'MINUTES'", change_newer_desc) do |change_newer| | ||
@options[:change_newer] = Integer(change_newer) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as the first glance i was wondering why there is an Integer and not a boolean here.
I think that the name change_newer_since
can remove all doubts, and we can accept the integer.
Otherwise, as an option i would expect a boolean for activating something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. I will change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great pr part of the inline comments,
i think we need also 2 things:
-
update the documentation with example and more verbose about this ( sd oemthing what you wrote in the pr)
-
we should have a spec test for this
7b99a0a
to
d8621fc
Compare
tests/spec/cmdline_spec.rb
Outdated
it 'gitarro should see at least PR #30 changed in the last 60 seconds' do | ||
context = 'changed-since' | ||
desc = 'changed-since' | ||
@comment = @rgit.create_comment(@pr, "gitarro rerun #{context} !!!") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you need here to target explicetly the PR number 30, you will write on the first pr open which is not the 30.
i
9bd3b20
to
660bec5
Compare
tests/spec/cmdline_spec.rb
Outdated
context = 'changed-since' | ||
desc = 'changed-since' | ||
@pr = @rgit.pr_by_number(PR_NUMBER) | ||
@comment = @rgit.create_comment(@pr, "gitarro rerun #{context} !!!") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here i would write instead of @comment comment
, since we don't need a class var
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, pending test and push.
tests/spec/cmdline_spec.rb
Outdated
it 'gitarro should see at least PR #30 changed in the last 60 seconds' do | ||
context = 'changed-since' | ||
desc = 'changed-since' | ||
@pr = @rgit.pr_by_number(PR_NUMBER) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same for pr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, pending test and push.
About the refactor: I can't do nothing but fully agree with you. I always say the same thing and this time I even broke my own rules. My mistake :-) That said, I think we should open an issue to refactor Backend class as a priority. It's growing and most probably we can simplify some stuff and move other stuff to a new class. In the end most of the refactors we are adding are to prevent rubocop from complaining about methods or classes being bigger than the rules allow. About all other changes: I will have a look and will fix. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some inline comments
gitarro.rb
Outdated
@@ -8,7 +8,9 @@ | |||
require_relative 'lib/gitarro/backend' | |||
|
|||
b = Backend.new | |||
prs = b.open_prs | |||
prs = b.open_newer_prs | |||
exit 0 if b.check_pr_list_empty(prs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
following issue #64 we can rename this as
prs_list_empty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, pending test and push.
lib/gitarro/backend.rb
Outdated
# Check if a PR list is empty | ||
# If it is and we do want to consider the update time, return true | ||
# Otherwise return false | ||
def check_pr_list_empty(prs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renaming
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, pending test and push.
lib/gitarro/backend.rb
Outdated
def open_newer_prs | ||
prs = [] | ||
@client.pull_requests(@repo, state: 'open').each do |pr| | ||
prs << pr if pr_last_update_less_than(pr, @options[:changed_since]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cosmetical :
https://stackoverflow.com/questions/10569529/ruby-difference-between-array-and-arraypush
I tend to prefer prs.push(pr) (since it is more verbose) but <<
it is ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, pending test and push.
lib/gitarro/backend.rb
Outdated
client.create_status(@repo, pr.head.sha, status, context: @context, | ||
description: @description, | ||
target_url: @target_url) | ||
true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think this true should go on the respectives public method
retrigger_pr_number
etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I placed this at the wrong place (create_status)
Shouldn't we place it a launch_test_and_setup_status since it's in use by all the public methods? In the end it's being used by all of them (with the exception of changelog) that are being used at /gitarro.rb
660bec5
to
04555a6
Compare
Done. Unit and acceptance tests passed locally. |
lib/gitarro/backend.rb
Outdated
return false | ||
end | ||
pr_all_files_type(@repo, pr.number, @file_type) | ||
return false unless @pr_files.any? | ||
return false unless pr_all_files_type(pr.number, @file_type).any? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: imho we can refactor this;
return pr_all_files_type(pr.number, @file_type).any?
Which will return false if there is no file and true if any
and we can remove line 294 true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can just refactor as:
pr_all_files_type(pr.number, @file_type).any?
Can't we?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
04555a6
to
55b34ef
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
last nitpick
lib/gitarro/backend.rb
Outdated
# public method for get prs opened and matching the changed_since | ||
# condition | ||
def open_newer_prs | ||
prs = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ntipick: we can refactor this with a beatiful ruby enumerable class.
prs = @client.pull_requests(@repo, state: 'open').select do
pr_last_update_less_than(pr, @options[:changed_since])
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$ irb
irb(main):001:0> [1,2,3].select {|n| n > 4 }
=> []
http://ruby-doc.org/core-2.2.2/Enumerable.html#method-i-select
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
…id exceeding GitHub API limits
55b34ef
to
5fa3a03
Compare
@juliogonzalez i think we can upload the new version in production with this. |
Sure, can you update the gem while I update the docker image? |
Maybe we should also create a release for each time we update the Gem |
yop, imho i would wait to create the gem until a week/3-4 days of running in production. ( in case there is a bug that we overlooked and so on that we need to fix) so in this way we prevent to publish buggy gems and releases. |
Ok, sounds fine. Updating Docker image and changing jobs. |
Since some people (including us) is using a cron-like pull instead of GitHub Webhooks, knowing which PRs were updated since the last time the cron ran a pull can avoid some request to the API (otherwise it is easy to hit the request limit easily -as we do now- and get failed tests just because we can't contact GitHub).
New parameter is
--changed_since <SECONDS>
If present it will consider only PRs with changes newer than X seconds.
So for example, if we look for PRs requiring tests each 5 minutes, and we set here
--changed_since=60
it will ignore open PRs unless there were changes in the last 60 seconss.A change is considered a commit or a comment, and it will update the field
updated_at
at GitHub API (if the PR was just opened thenupdated_at
has the same value ascreated_at
.If the parameter is NOT present, gitarro will work as usual.
If
SECONDS
is 0 then it will ignore all PRs. And negative values is the same as not using the parameters. These last two options are useful for testing, including unit tests.